fix: harden resolver, config ownership, and doc-test strictness#4
Conversation
WHY: CodeRabbit review of zigdoc#2 surfaced findings against ziglint's
vendored copy; the canonical home for those fixes is this repo.
WHAT:
- TypeResolver: cap alias-following at 32 hops (cyclic aliases like
'const a = b; const b = a;' no longer recurse unbounded); stop
coercing field access on user types into .std_type — return .unknown
so user namespaces cannot masquerade as stdlib ones.
- Config: add pub deinit(allocator) for the paths slice ownership;
re-export Rule publicly (Z012 — pub fns take it); deinit ends with
self.* = undefined per Z030.
- doc_comments: skip the full qualifier set (inline/extern/export/...)
when scanning backward, so 'pub inline fn' keeps its doc comment.
- doc_tests: reject unexpected diagnostics even when expectations are
present; run all fixtures before failing; declare the incidental
second rule in 5 affected fixtures (Z002+Z031, Z007+Z013, Z011+Z020,
Z021+Z009, Z031+Z002).
NOTE: ConfigType/RuleConfig stay PascalCase — type-returning functions
follow Zig's type-constructor naming convention.
IMPACT: linter precision (the user_type fix immediately exposed a real
Z012 in our own Config.zig, fixed here); public Config API gains deinit.
VALIDATION: mise x zig@0.16.0 zig build test --summary all =>
7/7 steps, 274/274 tests (125 doc fixtures now strict), fmt clean,
self-lint gate clean.
|
Warning Review limit reached
More reviews will be available in 13 minutes and 41 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR enhances the Zig linter's diagnostic accuracy and infrastructure. Config gains memory cleanup and public Rule re-export; TypeResolver bounds alias recursion and fixes field access resolution; doc comment scanning becomes qualifier-aware; doc tests validate strict expectation matching; rule documentation reflects the stricter diagnostics now detected. ChangesDiagnostic Accuracy and Infrastructure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR applies a set of hardening and correctness fixes across ziglint’s semantic resolver, configuration ownership, and documentation test runner, and updates rule docs to match the now-stricter doc-test expectations.
Changes:
- Add an alias-chain depth cap in
TypeResolverand avoid coercing user namespace field-access into stdlib type resolution. - Add
Config.deinit(allocator)for ownedpathscleanup and publicly re-exportRulefromConfig. - Make doc-tests fail on unexpected diagnostics (and aggregate failures), and improve doc comment scanning across declaration qualifiers.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/TypeResolver.zig | Adds alias depth limiting to avoid recursion issues; tightens field-access typing to prevent user/std namespace conflation. |
| src/doc_tests.zig | Enforces “no unexpected diagnostics” and aggregates doc-test failures across fixtures. |
| src/doc_comments.zig | Extends backward scan to skip common declaration qualifiers so doc comments attach correctly. |
| src/Config.zig | Exposes Rule publicly and adds deinit to free config-owned paths. |
| docs/rules/Z031.md | Adds a second expected diagnostic to match stricter doc-test validation. |
| docs/rules/Z021.md | Adds a second expected diagnostic to match stricter doc-test validation. |
| docs/rules/Z011.md | Adds a second expected diagnostic to match stricter doc-test validation. |
| docs/rules/Z007.md | Adds a second expected diagnostic to match stricter doc-test validation. |
| docs/rules/Z002.md | Adds a second expected diagnostic to match stricter doc-test validation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WHY: PR #4 review — the collect-all catch swallowed infrastructure errors (I/O, OOM) along with assertion failures, hiding root causes. WHAT: catch-switch counts MissingExpectedDiagnostic/UnexpectedDiagnostic toward failure_count and rethrows everything else. IMPACT: doc-test runner; no behavior change for passing suites. VALIDATION: zig build test --summary all => 274/274, self-lint clean.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/doc_tests.zig`:
- Around line 199-203: The loop in runAllDocTests currently swallows every error
from runDocTest by using a blanket catch, turning allocator/IO/runtime errors
into ordinary fixture failures; update the call site so that
runDocTest(allocator, full_path, doc_test, &tmp_dir) is invoked with try or
explicit error handling: if the returned error is a doc-test fixture mismatch
(the specific error enum/value your runDocTest returns for test failures) then
increment failure_count, but for allocator/IO/runtime or unexpected errors
propagate/return the error from runAllDocTests (or log and abort) instead of
incrementing failure_count; reference runDocTest, runAllDocTests, failure_count,
allocator, full_path, doc_test, and tmp_dir when making the change.
In `@src/TypeResolver.zig`:
- Around line 494-497: Rename the constant symbol max_alias_depth to
SCREAMING_SNAKE_CASE MAX_ALIAS_DEPTH and update every reference to it (e.g., any
logic in TypeResolver that checks or decrements alias depth while following
alias chains) so the identifier matches the new name; ensure the constant
declaration (formerly "const max_alias_depth = 32;") is renamed and all usages
are adjusted to MAX_ALIAS_DEPTH to satisfy the Zig constant naming convention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1eefc593-d175-49f1-8d3c-9a2a99ae87fb
📒 Files selected for processing (9)
docs/rules/Z002.mddocs/rules/Z007.mddocs/rules/Z011.mddocs/rules/Z021.mddocs/rules/Z031.mdsrc/Config.zigsrc/TypeResolver.zigsrc/doc_comments.zigsrc/doc_tests.zig
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
EugOT/dotfiles(manual)
WHY: PR #4 review asked for SCREAMING_SNAKE_CASE per AGENTS.md, but the repo's own Z006 rule (and Zig stdlib convention, e.g. max_path_bytes) enforces snake_case for value constants — the prose was stale, the machine rule is canonical. WHAT: max_alias_depth keeps snake_case (now with explicit u32 type); AGENTS.md naming section now documents snake_case value constants, PascalCase type-returning functions, and cites Z006. IMPACT: docs only; no behavior change. VALIDATION: zig build test => 274/274, self-lint clean (Z006 satisfied).
|
Declining the |
WHY: CodeRabbit threads on PR #2 flagged 7 findings inside the vendored ziglint copy; the fixes landed upstream (EugOT/ziglint#4) — patching the package cache in-place would desync it from its content hash. WHAT: build.zig.zon repinned to main@a2fa56a (hash t0bwLzCwBQA...); vendored zig-pkg snapshot refreshed, stale t0bwLz2XBQC... removed. Upstream brings: Config.deinit + pub Rule re-export, doc-comment qualifier scan (pub inline fn), strict doc-test gate, alias-recursion cap, user_type no longer coerced to std_type, findProjectRoot caller-allocator fix. IMPACT: zigdoc lint gate now runs the hardened ziglint. VALIDATION: mise x zig@0.16.0 zig build test --summary all => all steps green (tests, zig fmt --check, ziglint self-lint via new pin).
* Update for Zig 0.16 * fix: complete zig 0.16 quality gates * fix(zigdoc): address PR #2 review — escape JSON paths, propagate build-runner failures, plug initProject leak WHY: Review on #2 (Codex P2 ×2, Copilot ×1) flagged correctness + resource issues that let malformed input or build failures masquerade as missing docs. WHAT: - build_runner_0.16.zig: encode module/import names and root paths via std.json.Stringify.encodeJsonString instead of raw "{s}" interpolation, so Windows backslash paths (C:\...) no longer emit invalid JSON that breaks parseBuildOutput. - main.zig processBuildZig: return error.BuildRunnerFailed (with stderr) on non-zero build-runner exit instead of silently continuing with zero modules. - main.zig initProject: cap run() stdout/stderr at .limited(1MiB) and free the caller-owned buffers (was leaking on success path). - Walk.zig: migrate StringArrayHashMapUnmanaged -> array_hash_map.String (0.16). - build.zig.zon: repoint ziglint dep ref fix/0.16-build-compat -> main. IMPACT: zigdoc module discovery on Windows and on failing builds; no public API change. VALIDATION: mise x zig@0.16.0 zig build test --summary all => 13/13 steps, 15/15 tests, zig fmt --check clean, ziglint self-lint gate clean. * fix: lift PR 2 flake and runner to Zig 0.16 * chore(deps): bump ziglint to a2fa56a (review-hardening release) WHY: CodeRabbit threads on PR #2 flagged 7 findings inside the vendored ziglint copy; the fixes landed upstream (EugOT/ziglint#4) — patching the package cache in-place would desync it from its content hash. WHAT: build.zig.zon repinned to main@a2fa56a (hash t0bwLzCwBQA...); vendored zig-pkg snapshot refreshed, stale t0bwLz2XBQC... removed. Upstream brings: Config.deinit + pub Rule re-export, doc-comment qualifier scan (pub inline fn), strict doc-test gate, alias-recursion cap, user_type no longer coerced to std_type, findProjectRoot caller-allocator fix. IMPACT: zigdoc lint gate now runs the hardened ziglint. VALIDATION: mise x zig@0.16.0 zig build test --summary all => all steps green (tests, zig fmt --check, ziglint self-lint via new pin). --------- Co-authored-by: Tim Culverhouse <tim@timculverhouse.com>
…w DoS) (#5) * fix(TypeResolver): bound nodeIsTypeRef alias recursion (stack-overflow DoS) WHY: ziglint analyzes untrusted source. The nodeIsTypeRef / varDeclIsTypeRef / identifierIsTypeRef trio recursed with no depth guard, so a cyclic alias such as `const a = b; const b = a;` (or self-referential `const a = a;`) in any linted file caused unbounded recursion and a stack-overflow crash. PR #4's "harden resolver" added max_alias_depth=32 but only to findMethodInFileAsStructDepth, missing this sibling family. (Pre-existing upstream; introduced by upstream 6b11b20.) WHAT: Thread a `depth: u32` parameter through nodeIsTypeRef (via new nodeIsTypeRefDepth), varDeclIsTypeRef, identifierIsTypeRef, and fieldAccessIsTypeRef, incrementing on each recursive hop and returning false once depth >= max_alias_depth. Public isTypeRef() entry starts at depth 0. Mirrors the existing findMethodInFileAsStructDepth guard pattern. Added regression tests: cyclic alias, self-referential alias (both via isTypeRef), and a cyclic method alias (via findFnInCurrentModule, locking in the pre-existing guard). IMPACT: src/TypeResolver.zig only. No behavior change for non-cyclic input; the 32-hop cap is far beyond any real alias chain. Eliminates a remotely triggerable linter crash on adversarial source. VALIDATION: New tests crash (10001-frame stack overflow) before the fix and pass after. `zig build test --summary all` -> 7/7 steps, 277/277 tests pass, zig fmt clean, self-lint (run exe ziglint) clean. Found via multi-agent audit of the fork diff (bfcb30d..main) with adversarial verification. * fix(TypeResolver): count one depth increment per alias hop WHY: CodeRabbit and Copilot both flagged that the alias-recursion guard incremented depth twice per logical hop (identifierIsTypeRef -> varDeclIsTypeRef(depth+1) -> nodeIsTypeRefDepth(init, depth+1)), so max_alias_depth=32 effectively capped at ~16 alias links and could false-negative on long but acyclic alias chains. WHAT: identifierIsTypeRef now passes `depth` (not depth+1) into varDeclIsTypeRef; the single increment stays at the actual alias-following recursion in varDeclIsTypeRef. One alias link now costs exactly one increment, matching the intended "~32 hops". Termination is unchanged (every cycle still passes the incrementing call). IMPACT: src/TypeResolver.zig only. No safety change (still terminates, no overflow); removes a false-negative risk for deep legitimate alias chains. VALIDATION: zig build test --summary all -> 7/7 steps, 277/277 tests pass, fmt clean, self-lint clean. Cyclic/self-referential alias regression tests still pass.
Applies the substantive review findings CodeRabbit raised on EugOT/zigdoc#2 against the vendored ziglint copy — fixed here at the source instead of patching the package cache.
.user_typefield access no longer coerces into.std_type(returns.unknown), so user namespaces can't masquerade as stdlib ones. The precision gain immediately exposed a real Z012 in our ownConfig.zig— fixed below.pub fn deinit(allocator)(paths slice had no deinitializer);Rulere-exported publicly (Z012);deinitendsself.* = undefined(Z030).pub inline fnkeeps its docs.ConfigType/RuleConfig— type-returning functions are PascalCase by Zig convention.findProjectRootpage_allocator leak (landed in align: sync fork with upstream rockorager Zig 0.16 migration #2/fix: make ziglint self-lint clean #3; the vendored copy in zigdoc is just stale — a dep bump there follows this merge).Validation:
mise x zig@0.16.0 zig build test --summary all→ 7/7 steps, 274/274 tests, fmt clean, strict self-lint clean.